Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow jobs to have return values, which get persisted in the DB #103

Open
wants to merge 15 commits into
base: master
Choose a base branch
from

Conversation

saurabhnanda
Copy link
Owner

No description provided.

@saurabhnanda saurabhnanda marked this pull request as draft October 6, 2023 05:47
@saurabhnanda
Copy link
Owner Author

@jappeace @ivb-supercede any thoughts on this functionality / feature? Few notes:

  • makes sense only when completed jobs are deleted after a delay. This allows an end-user/UI to trigger a job, and poll for the job's result/output (who's usefulness is ephemeral in nature, i.e. you can afford to lose this result after X days, when the job is deleted)
  • enables one to write retryable worfklows of jobs (inspired by Temporal) -- which will be my next PR

@jappeace
Copy link
Collaborator

jappeace commented Oct 6, 2023

Yes this maybe usefull for quick n' dirty jobs where you don't want to make a dedicated table for the job to writeout in.

Good to see you back :)

@jappeace
Copy link
Collaborator

jappeace commented Oct 6, 2023

I left supercede, @RiugaBachi may also wish to comment.

@saurabhnanda
Copy link
Owner Author

Folks, this has a breaking change in the library's external API. Is everyone largely fine with this?

@jappeace @tfausak @tchoutri

(please tag other active users, as well)

@saurabhnanda saurabhnanda marked this pull request as ready for review October 9, 2023 08:17
@tchoutri
Copy link
Contributor

tchoutri commented Oct 9, 2023

No objection on my part

@jappeace
Copy link
Collaborator

jappeace commented Oct 9, 2023

Is there a way you could implement this without breaking existing code?

@tfausak
Copy link
Contributor

tfausak commented Oct 9, 2023

I see why this is useful. My team wouldn't necessarily benefit from it, so I could take or leave this change.

Any change to the schema of the job table is unfortunate. However adding a new optional column is about as low impact as you can get.

I would appreciate a non-breaking release before any breaking change. We've been using a source-repository-package (rather than a Hackage release) for a while now.

@saurabhnanda
Copy link
Owner Author

@jappeace @tfausak one way to not break any existing code would be to have a build-time flag along with liberal use of CPP at the following places:

  • OddJobs.Types.Job data type to add two extra fields if the CPP flag has been set. Two fields will be added if this flag is set:
    • jobResult
    • jobParentId
  • saveJobQuery & saveJobIO & runJob
  • defaultDelayedJobDeletion (added in Allow immediate & delayed job deletion #106 ), to ensure a child job is deleted only along with its parent
  • there could be more places where CPP will need to be used.

is there any other way to do this? and if CPP is the only way, when do we get finally get rid of the CPP?

@jappeace
Copy link
Collaborator

I was thinking along the lines of: instead of changing this function in place, we add a new one and see if it can be made compatable,
however I'm not sure if it's possible.

I'd not go the CPP/flag route as that incurs a rather troublsome maintenance burden and rather do the breakage.

@saurabhnanda
Copy link
Owner Author

I was thinking along the lines of: instead of changing this function in place, we add a new one and see if it can be made compatable,

@jappeace I think two versions of functions can still be dealt with, but what do you think about the Job{..} data type?

Does anyone use the Job{..} constructor in their application code? For creating a job if everyone is using the createJob function, then adding optional/nullable fields to Job{..} should not be. a problem...

@saurabhnanda
Copy link
Owner Author

I think the two DB columns that all users of the library are going to be forced to create are a greater problem than the Haskell code.... any ideas on how to tackle this?

@saurabhnanda
Copy link
Owner Author

@tchoutri @jappeace @tfausak what do you feel about the current attempt on making this backward compatibly and purely opt-in?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants